Improve redirect error message #460

bhrutledge · 2019-05-12T14:47:51Z

Closes #310

Adds a suggested course of action to the RedirectDetected error message in upload
Moves error message to exception class
Updates register to use the new message (with a test)
Adds conftest.py for reused fixtures

The original issue has a suggestion to avoid the redirect as a special case for PyPI. I would probably do that in utils.normalize_repository_url by appending a trailing slash if the URL is in _HOSTNAMES. However, it seemed like a more useful error message would benefit other repositories, and might be sufficient for the official ones.

codecov · 2019-05-12T14:49:53Z

Codecov Report

Merging #460 into master will increase coverage by 2.18%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #460      +/-   ##
==========================================
+ Coverage   80.74%   82.93%   +2.18%     
==========================================
  Files          14       14              
  Lines         748      750       +2     
  Branches      108      108              
==========================================
+ Hits          604      622      +18     
+ Misses        110       92      -18     
- Partials       34       36       +2

Impacted Files	Coverage Δ
twine/commands/register.py	`58.33% <100%> (+58.33%)`	⬆️
twine/commands/upload.py	`91.66% <100%> (+4.16%)`	⬆️
twine/exceptions.py	`100% <100%> (ø)`	⬆️
twine/wininst.py	`31.57% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da07f78...09aa4e2. Read the comment docs.

sigmavirus24 · 2019-05-12T23:21:31Z

twine/exceptions.py

+    @classmethod
+    def from_args(cls, repository_url, redirect_url):
+        msg = "\n".join([
+            "Unexpected redirect from {0} to {1}."


I don't think we support 2.6 any longer meaning you can drop the numbers here, if you'd like.

sigmavirus24 · 2019-05-12T23:25:29Z

twine/exceptions.py

+        msg = "\n".join([
+            "Unexpected redirect from {0} to {1}."
+            .format(repository_url, redirect_url),
+            "You might need to change the configured repository URL.",


In the more common cases a redirect here is innocuous and perhaps changing the URL is fine. But there could be cases where folks are being redirected to a malicious URL that will be able to:

steal credentials

store the uploaded package data

In that case, I don't want folks to think "Geeze, really?! I have to copy and paste this into my pypirc for this stupid tool to work?! WTF IS WRONG WITH THOSE PEOPLE" and then to angrily and carelessly copy and paste the malicious URL.

Granted, I think the this is very unlikely, but still I would like to see more cautious wording. Something like:

You should examine the URLs carefully and if you trust them, update your configured repository URL.

Also, would you share your reasoning for using a newline to separate these lines? Has the project moved towards preferring newlines?

Thanks for the feedback. I wordsmithed the error message a bit in 1b80776. Re: newlines, I thought it was more readable, and there are a couple instances of them being used in the project. But I don't feel strongly, and am happy to change it.

twine/twine/exceptions.py

Lines 50 to 60 in da07f78

@classmethod

def from_args(cls, target_url, default_url, test_url):

"""Return an UploadToDeprecatedPyPIDetected instance."""

return cls("You're trying to upload to the legacy PyPI site '{}'. "

"Uploading to those sites is deprecated. \n "

"The new sites are pypi.org and test.pypi.org. Try using "

"{} (or {}) to upload your packages instead. "

"These are the default URLs for Twine now. \n More at "

"https://packaging.python.org/guides/migrating-to-pypi-org/"

" .".format(target_url, default_url, test_url)

)

twine/twine/utils.py

Lines 122 to 132 in da07f78

except KeyError:

msg = (

"Missing '{repo}' section from the configuration file\n"

"or not a complete URL in --repository-url.\n"

"Maybe you have a out-dated '{cfg}' format?\n"

"more info: "

"https://docs.python.org/distutils/packageindex.html#pypirc\n"

).format(

repo=repository,

cfg=config_file

)

bhrutledge · 2019-06-01T11:18:21Z

@sigmavirus24 Is there more that I can/should do with this pull request?

sigmavirus24

Sorry for the delay @bhrutledge I'm not really spending time on F/OSS and it was purely chance that I noticed this in my email today. It may be best to work with another maintainer on this going forward.

sigmavirus24 · 2019-06-01T14:47:03Z

tests/test_upload.py

+        upload.upload(upload_settings, [WHEEL_FIXTURE])
+
+    assert err.value.args[0] == (
+        "https://test.pypi.org/legacy attempted to redirect"


This is something I struggle with often. As a CLI, our user interface is what we print out. With that in mind, tests like these quickly become frustrating and feel overly strict when updating messages/message formatting/etc. I think testing that we're showing the correct data is important, but I also don't want us to test that we always show this message. I can imagine a future where we use internationalization to improve the experience and someone contributing from a locale that isn't en_* sees a test failure here too.

What do you think are the important parts of this message? What should we test for and what can we eliminate from this assertion?

Thanks, @sigmavirus24. In this case, I followed the pattern of the existing tests, but in generally I agree with your opinion. To that end, in f326758 I modified the assertions to check for what I think are the important bits, like URLs.

bhrutledge · 2019-06-02T21:55:13Z

@theacodes or @di: in light of this comment from @sigmavirus24, and as the other maintainers that I've interacted with, any thoughts on this PR?

I'm not really spending time on F/OSS and it was purely chance that I noticed this in my email today. It may be best to work with another maintainer on this going forward.

di · 2019-06-03T22:08:37Z

Just a heads up, I'm not technically a twine maintainer, but @theacodes or @jaraco are.

tests/test_upload.py

bhrutledge added 2 commits May 12, 2019 10:34

Improve redirect message

e53c9b5

Update redirect message for registration

d98c1dd

sigmavirus24 reviewed May 12, 2019

View reviewed changes

Update error message

1b80776

sigmavirus24 reviewed Jun 1, 2019

View reviewed changes

Make error message tests less brittle

f326758

jaraco reviewed Jun 8, 2019

View reviewed changes

tests/test_upload.py Show resolved Hide resolved

jaraco approved these changes Jun 8, 2019

View reviewed changes

Update changelog

09aa4e2

jaraco merged commit add0aea into pypa:master Jun 8, 2019

bhrutledge mentioned this pull request Jul 7, 2019

Adds a type check to ensure the description type provided is valid #473

Closed

bhrutledge mentioned this pull request Oct 9, 2019

leaving out a trailing slash in the pypi URL causes twine to fail with a 410 #310

Closed

bhrutledge mentioned this pull request Sep 14, 2021

Having a new check to avoid redirects from PyPI #809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve redirect error message #460

Improve redirect error message #460

bhrutledge commented May 12, 2019

codecov bot commented May 12, 2019 •

edited

Loading

sigmavirus24 May 12, 2019

sigmavirus24 May 12, 2019

bhrutledge May 18, 2019

bhrutledge commented Jun 1, 2019

sigmavirus24 left a comment

sigmavirus24 Jun 1, 2019

bhrutledge Jun 2, 2019

bhrutledge commented Jun 2, 2019

di commented Jun 3, 2019

	@classmethod
	def from_args(cls, target_url, default_url, test_url):
	"""Return an UploadToDeprecatedPyPIDetected instance."""
	return cls("You're trying to upload to the legacy PyPI site '{}'. "
	"Uploading to those sites is deprecated. \n "
	"The new sites are pypi.org and test.pypi.org. Try using "
	"{} (or {}) to upload your packages instead. "
	"These are the default URLs for Twine now. \n More at "
	"https://packaging.python.org/guides/migrating-to-pypi-org/"
	" .".format(target_url, default_url, test_url)
	)

	except KeyError:
	msg = (
	"Missing '{repo}' section from the configuration file\n"
	"or not a complete URL in --repository-url.\n"
	"Maybe you have a out-dated '{cfg}' format?\n"
	"more info: "
	"https://docs.python.org/distutils/packageindex.html#pypirc\n"
	).format(
	repo=repository,
	cfg=config_file
	)

Improve redirect error message #460

Improve redirect error message #460

Conversation

bhrutledge commented May 12, 2019

codecov bot commented May 12, 2019 • edited Loading

Codecov Report

sigmavirus24 May 12, 2019

Choose a reason for hiding this comment

sigmavirus24 May 12, 2019

Choose a reason for hiding this comment

bhrutledge May 18, 2019

Choose a reason for hiding this comment

bhrutledge commented Jun 1, 2019

sigmavirus24 left a comment

Choose a reason for hiding this comment

sigmavirus24 Jun 1, 2019

Choose a reason for hiding this comment

bhrutledge Jun 2, 2019

Choose a reason for hiding this comment

bhrutledge commented Jun 2, 2019

di commented Jun 3, 2019

codecov bot commented May 12, 2019 •

edited

Loading